-
Notifications
You must be signed in to change notification settings - Fork 208
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
DO NOT MERGE - just commentary and questions as I read #4553
Conversation
"src/**/*.js", | ||
"test/**/*.js", | ||
"exported.js", | ||
"globals.d.ts", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is reformatted only because I tried an experiment of adding `"bundles/install-on-chain.js" to this list, so it would no longer be exempt from linting.
// This should have worked, but was passed something non-hardened | ||
// export const allValues = deeplyFulfilled; | ||
// So I tried the following instead | ||
export const allValues = obj => deeplyFulfilled(harden(obj)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left this uncommented for now, and lines 37-49 commented above, so the resulting failures will appear under CI. See comment below for the bug causing these failures.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that this harden
masks, rather than fixes, bugs elsewhere. The passing around of those unhardened objects may have already caused problems before they reach here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
deeplyFulfilled loses the types of its args. So when I know I'm dealing with a Record<string, T>
, I much prefer allValues
.
I can work on the harden
aspects as well, though.
@@ -167,6 +167,7 @@ export const setupAmm = async ({ | |||
E(instanceAdmin).update('ammGovernor', g.instance), | |||
]); | |||
}; | |||
harden(setupAmm); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not surprised to see lots of functions exported but not hardened. Fortunately it is not very dangerous. But it is a hazard we need to avoid. It leaves mutable top-level state, such that two parties importing the same function from the same module instance can now communicate without calling the function. This isn't POLA and breaks reasoning by separation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it is a hazard we need to avoid
Could missing harden
be detected automatically? i.e. are there consistent rules for when to use harden or not?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, there are consistent rules:
all objects made by literal expressions (object literals, array literals, the many forms of function literals) must be tamper-proofed with harden before they can be aliased or escape from their static context of origin -- https://github.com/endojs/Jessie#must-freeze-api-surface-before-use
Automatic detection is an open research problem, IIUC. agoric-labs/jessica#35
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- a) One abstract rule is that an object should not escape the abstraction it is born in without first being hardened.
- b) A related rule is that module instances should be pure --- they should have no mutable top level state.
On #b, We have tried several times to write down static rules that would enforce these abstract rules. I think @michaelfig has grammatical restrictions for pure Jessie modules somewhere. @michaelfig ?
On #a, a conservative rule that works almost all the time is to just put a harden(...)
or Far(..., ...)
around the outer literal (object literal, array literal, function literal, class literal, regexp literal) where the object is first born. Then, no one gets to see the object in its pre-hardened state.
Occasionally, we do have valid code where we locally mutate the object before hardening it, but still harden it before it might escape. To enforce that statically would involve treating all variables holding the object as "owning" variables in the reference-capability sense. Or as linear or affine variables or something. The key is that we statically know that the object hasn't escaped via an alias before it's been hardened. We have sketched static rules for this but never got very far. Fortunately, the need for such delayed hardening happens even more rarely than I expected. We can almost always simply follow the conservative harden-the-literal rule above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On #b, a conservative rule is to only have top level const declarations of hardened things. Again, this conservative rule works almost always.
For exported functions, we follow a pattern of a slightly delayed harden:
export const foo = x => x * x;
harden(foo);
This still hardens foo
before anything could observe foo
in its unhardened state.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @dckc . I answered before I saw your response.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would an imperfect but worthwhile rule be that named exports from a module have a corresponding harden
? We could pretty easily lint for that.
E..g,
FAIL
export const sayHi = () => {
console.log("hi");
};
PASS
export const sayHi = () => {
console.log("hi");
};
harden(sayHi);
// eslint-disable-next-line endo/hardened-exports
export const foo = {};
EDIT: I also was typing while @erights was. If I'm reading "this conservative rule works almost always" right then the answer is yes that we could have a lint rule for that.
When it doesn't work how obvious is it? Would someone writing the module know to be able to work around it or suppress the lint? Or would might they ship something overly hardened and break downstream. (With or without the lint we need developers to know when to harden, so I don't suppose linting could be any worse and it should help both educate devs and help catch things they missed.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You may need to accept harden
or Far
there for the rule.
// says the average Gregorian year is 365.2425 | ||
// whereas the astronomical figure (tropical year) is 365.2422 | ||
// No mention made of the occasional leap second! | ||
// Whether we care depends on what we do with it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll admit this is overly pedantic and probably doesn't matter at all. But I am curious if, for our use, there is an unambiguous right answer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had wondered the same thing. It's a negligible difference and for loans the regulations allow treating interest in a leap year as any other.
https://www.consumerfinance.gov/rules-policy/regulations/1030/interp-7/#:~:text=Leap%20year.,earn%20interest%20for%20February%2029.
- Leap year. Institutions may apply a daily rate of 1/366 or 1/365 of the interest rate for 366 days in a leap year, if the account will earn interest for February 29.
@@ -76,12 +76,12 @@ export const setupAMMBootstrap = async ( | |||
produce.nameAdmins.resolve(nameAdmins); | |||
|
|||
/** @type {Record<string, Promise<{moduleFormat: string}>>} */ | |||
const governanceBundlePs = { | |||
const governanceBundlePs = harden({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an example of a missing harden
bug that's masked by the last version of allValues
, where it auto-hardens its argument. The second version of allValues
which is just an alias for deeplyFulfilled
would fail immediately when applied to this unhardened record, quickly revealing rather than masking the bug.
5e9c02a
to
bae910d
Compare
Stale! Closing. |
No description provided.